Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(design-system): add Input, Textarea component and stories #2246

Merged
merged 22 commits into from
Jul 7, 2021

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Jun 24, 2021

Problem

This PR adds the components/Input component alongside with their stories. A Playground story is also created to check how it could work with React hook form.

Closes #2002

Solution

Features:

  • add Input component and stories
  • add Textarea component and stories

Before & After Screenshots

In storybook

@karrui karrui force-pushed the form-v2/develop branch from 9d2de8c to 11ca404 Compare June 24, 2021 06:02
@karrui karrui force-pushed the form-v2/input-component branch from 5f5f818 to 4512c81 Compare June 24, 2021 06:29
@karrui karrui changed the title feat(design-system): add Input component and stories feat(design-system): add Input, Textarea component and stories Jun 24, 2021
karrui added 8 commits June 28, 2021 17:31
This is used in by Chakra's `InputGroup` component to remove border radii when paired with `InputLeftAddon` or `InputRightAddon`
this removes the need to have a per-input component memoization theme override
since they follow input state, no reason to not include those
also removed redundant isSuccess check
so react won't throw an error on the console
frontend/src/components/Textarea/Textarea.tsx Show resolved Hide resolved
Comment on lines +33 to +35
if (!props.isSuccess) {
return <ChakraInput ref={ref} {...inputProps} sx={inputStyles.field} />
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry why are we doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the comments in the non-success case, InputGroup is required for InputRightElement to retrieve the correct style props and will crash if not included.

However, since we just display the normal input on success, we do not need that special requirement and can just return the input as per normal

karrui added 3 commits July 5, 2021 20:02
# Conflicts:
#	frontend/src/assets/icons/BxsCheckCircle.tsx
#	frontend/src/theme/components/index.ts
# Conflicts:
#	frontend/package-lock.json
#	frontend/package.json
#	frontend/src/theme/components/index.ts
wrong indentation sorry
@karrui karrui requested a review from mantariksh July 7, 2021 02:51
@karrui karrui requested a review from tshuli July 7, 2021 02:51
boxShadow: 'none',
borderColor: 'primary.500',
borderWidth: '2px',
outline: (props: Record<string, any>) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

@karrui karrui merged commit f9f9162 into form-v2/develop Jul 7, 2021
@karrui karrui deleted the form-v2/input-component branch July 7, 2021 07:46
Comment on lines +40 to +45
<InputGroup>
<ChakraInput ref={ref} {...inputProps} sx={inputStyles.field} />
<InputRightElement sx={inputStyles.success}>
<Icon as={BxsCheckCircle} />
</InputRightElement>
</InputGroup>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could i clarify on why we chose to do this? Testing on chakra's website seems to suggest that there are styling conflicts and consumers of our component might mistakenly think that this is a base Input in success state and compose it as part of an InputGroup only to discover that it's actually not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants